[Refactoring] Create a new "AI" vertical#1195
Conversation
paulpetit-gg-ext
changed the title
paulpetit-gg-ext
force-pushed
the
ppetit/-/ai-vertical
branch
from
e55bffc to
110685e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1195 +/- ##
==========================================
- Coverage 92.85% 92.85% -0.01%
==========================================
Files 168 170 +2
Lines 8178 8198 +20
==========================================
+ Hits 7594 7612 +18
- Misses 584 586 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
paulpetit-gg-ext
force-pushed
the
ppetit/-/ai-vertical
branch
2 times, most recently
from
0d5cfd1 to
66bc455
Compare
There was a problem hiding this comment.
The coupling between the ai and secret vertical doesn't feel right to me. I wonder if we rather should move ggshield/verticals/secret/secret_scan_collection.py and ggshield/verticals/secret/secret_scanner.py into ggshield/core/scan and this file into the ai vertical. That way we could ensure a clean separation between verticals.
There was a problem hiding this comment.
I was talking to Aurélien about this and he had some interesting insights: The verticals represent GitGuardian business verticals (although auth is a pretty wide stretch for a vertical 😅). Therefore, the AI hooks' logic should reside in the secret vertical.
There was a problem hiding this comment.
Interesting. I suppose leaving the hook part in secret/ makes sense, but there are still a few questions:
- Adding an
ggshield aisubcommand is still relevant for us (for instance we are developing aggshield ai discovercommand that lists agents and MCP servers configured on a developer's machine and which wouldn't make much sense underggshield secret). Does it mean we can create the vertical or should we have a new command subgroup but not a newvertical/subfolder? - In case we have part of the new AI governance code under a specific folder, it means two things for the hook code:
- either it imports model from it (coupling)
- either everything useful for hooks stays under
secret/but we may have code duplication, and the logic specific to a given agent like Cursor is split between multiple folders.
I think in this case some form of coupling may be warranted. What do you think?
There was a problem hiding this comment.
After discussion with @agateau-gg , I pushed a new solution: both verticals are now independent, and the commands are able (with documented exceptions in the import linter) to pull the logic they need from both verticals (scan and ai).
paulpetit-gg-ext
force-pushed
the
ppetit/-/ai-vertical
branch
from
66bc455 to
7aaeac2
Compare
paulpetit-gg-ext
force-pushed
the
ppetit/-/ai-vertical
branch
from
7aaeac2 to
7474d2b
Compare
paulpetit-gg-ext
force-pushed
the
ppetit/-/ai-vertical
branch
from
7188a65 to
17f9363
Compare
6d7a
left a comment
6d7a
left a comment
There was a problem hiding this comment.
LGTM. Thank you for the changes!
3 participants
Context
Because we will add new AI governance capabilities to GIM, this PR is a refactoring that extracts what has been done for AI hooks into a new
aivertical that will be expanded upon later (new commands will arrive in theaivertical, and hooks won't only serve to scan secrets).It is quite a lot of change, but this is 99% moving code from a place to another and fixing imports.
I think it is worth having it in a distinct PR so that later work is more easily reviewed.
There is only one notable change that is not purely code shuffling:
Flavorhas been renamed to the less crypticAgentand has become an abstract class. The rationale is to keep everything specific to an AI agent (Cursor, Claude Code, etc.) into a single class/file even when the number of AI governance features will grow.Note that we still need at some point to be able to reuse both logic from the scan vertical (to scan secrets) and from the new ai vertical (for hooks, payload model, etc.).
Instead of adding coupling between verticals (ai -> scan or scan -> ai), this PR chose to have commands import what they need from each vertical in order to work. This is what I think maintains the clearer boundary between modules.
PR check list
skip-changeloglabel has been added to the PR.